Skip to content

Conversation

@jhlegarreta
Copy link
Contributor

@jhlegarreta jhlegarreta commented Sep 27, 2025

Generalize iterators to accept bvals or uptake as keyword arguments. The previous implementation of the linear, random and centralsym iterators was only accepting bvals. This patch set allows these iterators to work with PET data through the uptake argument.

Transition to keyword argument-only style. Adapt the doctests accordingly.

Document the functions by explicitly assigning the docstring to the __doc__ property of each function so that the SIZE_KEYS_DOC can be reused and to allow the examples be run by the doctring tests.

@jhlegarreta jhlegarreta force-pushed the enh/generalize-iterators branch from 08c29dc to 80de628 Compare September 27, 2025 21:06
@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Sep 27, 2025

Partially addresses #230. It remains the question about the bvalue and uptake iterators: #230 (comment).

@codecov
Copy link

codecov bot commented Sep 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.22%. Comparing base (aa0fcc8) to head (a1310e6).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #238      +/-   ##
==========================================
+ Coverage   76.08%   76.22%   +0.13%     
==========================================
  Files          28       28              
  Lines        1777     1779       +2     
  Branches      187      182       -5     
==========================================
+ Hits         1352     1356       +4     
+ Misses        362      361       -1     
+ Partials       63       62       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd make it more generic, including size in it, and then have SIZE_KEYS be a priority list (a bit like it used to be).

Exception then would be raised if size cannot be inferred.

WDYT?

@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Oct 27, 2025

From #243 (comment)

Will add the uptake kwarg tests once PR #238 gets merged.

Done in this PR.

WDYT?

Looks OK to me. Now, the size parameter is redundant and will only have kwargs, right? e.g.

def linear_iterator(size: int | None = None, **kwargs) -> Iterator[int]:
    if size is None:
        size = _get_size_from_kwargs(kwargs)

    return (s for s in range(size))

should be

def linear_iterator(**kwargs) -> Iterator[int]:
    size = _get_size_from_kwargs(kwargs)

    return (s for s in range(size))

and so on for the rest of the iterators.

Will amend once we agree on that.

@jhlegarreta jhlegarreta force-pushed the enh/generalize-iterators branch from d8b1e70 to 64492e2 Compare October 27, 2025 21:53
@oesteban
Copy link
Member

Now, the size parameter is redundant and will only have kwargs, right?

Correct, that was the thinking.

@jhlegarreta jhlegarreta force-pushed the enh/generalize-iterators branch 7 times, most recently from c271881 to 76f551e Compare October 28, 2025 08:06
Generalize iterators to accept `bvals` or `uptake` as keyword arguments.
The previous implementation of the linear, random and centralsym
iterators was only accepting `bvals`. This patch set allows these
iterators to work with PET data through the `uptake` argument.

Transition to keyword argument-only style. Adapt the doctests
accordingly.

Document the functions by explicitly assigning the docstring to the
`__doc__` property of each function so that the `SIZE_KEYS_DOC` can be
reused and to allow the examples be run by the doctring tests.

Co-authored-by: Oscar Esteban <[email protected]>
@jhlegarreta jhlegarreta force-pushed the enh/generalize-iterators branch from 76f551e to a1310e6 Compare October 28, 2025 08:14
@jhlegarreta jhlegarreta merged commit 8a38940 into nipreps:main Oct 28, 2025
10 checks passed
@jhlegarreta jhlegarreta deleted the enh/generalize-iterators branch October 28, 2025 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants